-
Notifications
You must be signed in to change notification settings - Fork 95
Calorimeter digi mixing #1678
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Calorimeter digi mixing #1678
Conversation
|
Hi @edcallaghan,
which require these tests: build. @Mu2e/write, @Mu2e/fnalbuild-users have access to CI actions on main. ⌛ The following tests have been triggered for b3cee8b: build (Build queue - API unavailable) |
|
☔ The build is failing at b3cee8b.
N.B. These results were obtained from a build of this Pull Request at b3cee8b after being merged into the base branch at 5d5efb9. For more information, please check the job page here. |
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for ae1489f: build (Build queue - API unavailable) |
|
☔ The build is failing at ae1489f.
N.B. These results were obtained from a build of this Pull Request at ae1489f after being merged into the base branch at 5d5efb9. For more information, please check the job page here. |
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for fb026e0: build (Build queue - API unavailable) |
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for 479180c: build (Build queue - API unavailable) |
|
☀️ The build tests passed at 479180c.
N.B. These results were obtained from a build of this Pull Request at 479180c after being merged into the base branch at 5d5efb9. For more information, please check the job page here. |
|
@bechenard I've requested your review based on authorship of existing code, but obviously please delegate/defer as appropriate. The real calorimeter action is in Also note that f56bda5 patches what I think is a (very minor) bug in the digitization which allows the simulation to produce adc readings which are one count above the true max reading, but I'll reverse that if I misunderstood. |
|
In the CI report, check_cmake is reporting a problem, please take a look at that. Thanks |
|
@FNALbuild run build test |
|
⌛ The following tests have been triggered for 71a8ca4: build (Build queue - API unavailable) |
|
☀️ The build tests passed at 71a8ca4.
N.B. These results were obtained from a build of this Pull Request at 71a8ca4 after being merged into the base branch at 5d5efb9. For more information, please check the job page here. |
|
@rlcee thanks, I forgot to propagate new classes into |
bechenard
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Ed, very nice code, elegant and well written. I just have a suggestion, the rest should work.
More background on the MC truth association. The issue with the Calo is that some Simparticles won't lead to a reconstructed signal, and several Simparticles might contribute to the same shower. We also needed to reduce the data volume drastically. My solution was to concatenate the MC information into CaloShowerSim objects describing the energy deposited by each SimParticle in a given crystal (daughter of CaloShowerStep objects (energy deposited by SimParticle by Geant4 steps) and parent of CaloShowerRO object ("hits" in the SiPMs)). This step is performed before the creation of any caloDigi. Then we try to match the reconstructed digis to the CaloShowerSim objects by comparing their times. Downstream MC information is just a convenient summary of these CaloShowerSim objects.
If you have several sources of CaloShowerSim objects, you need to merge them into a single collection before the truth association is performed (incidentally, you need to save the relevant SimParticles, CaloShowerSteps, and CaloShowerRO). I don't recall exactly when your salting is performed, but if this is done before the MC truth association then all should be fine.
Is there a point regarding MC that I misunderstood from your code?
|
|
||
| protected: | ||
| const CaloDigi _digi; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to have this as a protected member instead of private? (I'll admit I;m a bit lazy and don;t want to read all the mixing code...)
| // next, (approximately) sort all subsets via time, to avoid | ||
| // pathological situations where timing buckets are misconstructed | ||
| // we shamefully and begrudgingly do this indirectly via bare pointers | ||
| std::map<SiPMID_t, CaloDigiWrapperCollection> wrappers_map; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose we can begrudgingly accept this solution...
| for (size_t i = 0 ; i < wrappers.size() ; i++){ | ||
| sorted.Append(*sortable[i]); | ||
| } | ||
| wrappers_map.emplace(id, sorted); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this make a copy of the sorted wrapper objects? If so, there might be no need for pointers, could you just make a copy and sort them without pointers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, this is a half stupid comment since sorting objects will take longer than pointer. However, it is always a bad idea to use pointer to objects in a vector because they get invalidated if the vector is reallocate, and this is a nasty bug to track... You should be safe because the object has fixed length, but I don;t know enough about c++ to make sure this will work for sure. So maybe for the sake of showing the good example we should avoid pointers?
This PR implements digi mixing for the calorimeter. The mechanics largely follow the pattern set by the tracker: the relevant data products are mixed using standard technology, and digis from the same channel which overlap in time are resolved into a single digi. For calorimeter digis, which are principally waveforms, this is conceptually straightforward: the waveforms are summed. It will, however, require extra effort to retain MC info for the calorimeter: unlike the tracker or CRV, which instantiate all MC info at the time of digi creation, it seems that some MC info for the calorimeter is reconstructed, making use of primary
SimParticleinfo, after digis have already been made and interpreted as hits. That should be fine, but it require extra book-keeping/massaging to work in situations where there is MC info (i.e. detector data) or some digis were digitized independently of the simulated signal (e.g. digi mixing with conventional pileup).New classes:
CaloDigiWrapper: A thin wrapper for calorimeter digis which is meant to collapse multiple loops, i.e. over synchronized digi + mc info collections, into a single loop. MC info is yet not included, for reasons described above.CaloDigiWrapperCollection: stl-vector of the above, with additional logic for resolving a set of overlapping digis into one.Changes to existing functionality:
Mu2eProductMixer: Include provisions for mixingCaloDigis andCaloShowerSims, which (it seems to me) is the principal data product, not already mixable, which will allow to retain MC info.TimeBasedBuckets: Generalized from a sequence of fixed-length timing windows to a sequence of variable-length timing windows. This is necessary as waveforms from the calorimeter of of variable length.Changes to existing modules:
MergeDigis: Resolve collisions between overlapping calorimeter digis usingCaloDigiWrapperCollectionfunctionality.Thoughts looking forward: The simple sum of multiple waveforms into a single waveform admits two subtleties: the saturation of the ADC must be reevaluated, and any analog noise present in the waveform is now "counted" multiple times in the sum.
The former is accounted for in this PR, albeit with the ADC bit depth configuration now duplicate, as it is supplied directly to the
CaloDigiMakermodule, and not factored into some kind of conditions class. It would be good for that to be done, or maybe move the definition to fcl-prolog, to avoid the possibility of them getting out of sync.The latter is not accounted for, as currently an implemented waveform is just that: a waveform; it does not know "what" it contains. Conceptually, one can imagine ways of keeping the noise contribution to waveforms distinct until it absolutely needs to be added to any signal present, but that is outside of the scope of this PR and I'm not in a position to recommend something right now. But it's something we'll want to get right, since potential applications include mixing digis from the detector with both detector data and MC, both in situations with overlapping digis and not, so artefacts associated with different levels of noise are possible.